Conversation
There was a problem hiding this comment.
Pull request overview
This PR improves the CLI calculator’s diagnostics by adding token position tracking end-to-end and rendering user-facing errors with a caret indicator in the CLI.
Changes:
- Add
positionto tokenizerTokenand propagate it through parsing/evaluation to produceError at position N: ...diagnostics. - Update CLI entrypoint to run tokenize/parse/eval phases explicitly and render position-aware errors with a caret via a new display utility module.
- Add/adjust CLI tests to cover new error messages and edge cases (empty expression, division by zero, negative sqrt, etc.).
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/cli/test_tokenizer.mojo | Updates empty-input behavior to expect an error. |
| tests/cli/test_error_handling.mojo | Adds a new test suite covering malformed expressions and math edge cases, including position substrings. |
| src/cli/main.mojo | Refactors CLI execution to tokenize/parse/evaluate explicitly and render errors with caret/colour output. |
| src/cli/calculator/tokenizer.mojo | Adds token position tracking and emits position-aware tokenizer errors (plus empty-expression handling). |
| src/cli/calculator/parser.mojo | Emits position-aware parse errors for commas and mismatched parentheses. |
| src/cli/calculator/evaluator.mojo | Emits position-aware runtime/math errors (division by zero, missing operands, domain errors). |
| src/cli/calculator/display.mojo | New module for colored stderr output and caret rendering. |
| src/cli/calculator/init.mojo | Re-exports display helpers from the calculator package. |
| docs/plans/cli_calculator.md | Updates the plan document with ArgMojo v0.2.0 upgrade task notes and reindexes Phase 3 items. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Each token records its 0-based column position in the source | ||
| expression so that downstream stages can emit user-friendly | ||
| diagnostics that pinpoint where the problem is. | ||
|
|
||
| Raises: | ||
| Error: On empty/whitespace-only input, unknown identifiers, or | ||
| unexpected characters, with the column position included in | ||
| the message. |
There was a problem hiding this comment.
Tokenizer docstring says errors include a column position in the message, but the empty/whitespace-only case currently raises just Empty expression without the Error at position N: prefix. Either update the docstring to reflect this special case, or include position info (e.g. position 0) for empty expressions for consistency.
| """Empty string should raise an error since Phase 3.""" | ||
| var raised = False | ||
| try: | ||
| var toks = tokenize("") |
There was a problem hiding this comment.
var toks = tokenize("") is never used. For consistency with the other tokenizer error tests (and to avoid unused-variable warnings if enabled), assign to _ instead of creating an unused toks binding.
| var toks = tokenize("") | |
| _ = tokenize("") |
|
|
||
| from calculator import evaluate | ||
| from calculator.tokenizer import tokenize | ||
| from calculator.parser import parse_to_rpn |
There was a problem hiding this comment.
parse_to_rpn is imported but never used in this test module. Please remove the unused import to keep the test file clean (and to avoid failing builds if unused-import checks are enabled).
| from calculator.parser import parse_to_rpn |
| from sys import exit | ||
|
|
||
| from argmojo import Arg, Command | ||
| from calculator import evaluate |
There was a problem hiding this comment.
evaluate is imported but no longer used (the CLI now calls tokenize/parse_to_rpn/evaluate_rpn directly). Please drop the unused from calculator import evaluate import.
| from calculator import evaluate |
This PR improves the CLI calculator’s diagnostics by adding token position tracking end-to-end and rendering user-facing errors with a caret indicator in the CLI. **Changes:** - Add `position` to tokenizer `Token` and propagate it through parsing/evaluation to produce `Error at position N: ...` diagnostics. - Update CLI entrypoint to run tokenize/parse/eval phases explicitly and render position-aware errors with a caret via a new display utility module. - Add/adjust CLI tests to cover new error messages and edge cases (empty expression, division by zero, negative sqrt, etc.).
This PR improves the CLI calculator’s diagnostics by adding token position tracking end-to-end and rendering user-facing errors with a caret indicator in the CLI.
Changes:
positionto tokenizerTokenand propagate it through parsing/evaluation to produceError at position N: ...diagnostics.